Skip to content

CompressedImageSaver improvements#23567

Draft
JMS55 wants to merge 23 commits intobevyengine:mainfrom
JMS55:compressed-image-saver2
Draft

CompressedImageSaver improvements#23567
JMS55 wants to merge 23 commits intobevyengine:mainfrom
JMS55:compressed-image-saver2

Conversation

@JMS55
Copy link
Copy Markdown
Contributor

@JMS55 JMS55 commented Mar 29, 2026

Objective

  • Describe the objective or issue this PR addresses.
  • If you're fixing a specific issue, say "Fixes #X".

Closes #14671.

Solution

  • Describe the solution used to achieve the objective above.

Testing

  • Did you test these changes? If so, how?
  • Are there any parts that need more testing?
  • How can other people (reviewers) test your changes? Is there anything specific they need to know?
  • If relevant, what platforms did you test these changes on, and are there any important ones you can't test?

Showcase

This section is optional. If this PR does not include a visual change or does not add a new feature, you can delete this section.

  • Help others understand the result of this PR by showcasing your awesome work!
  • If this PR adds a new feature or public API, consider adding a brief pseudo-code snippet of it in action
  • If this PR includes a visual change, consider adding a screenshot, GIF, or video
    • If you want, you could even include a before/after comparison!
  • If the Migration Guide adequately covers the changes, you can delete this section

While a showcase should aim to be brief and digestible, you can use a toggleable section to save space on longer showcases:

Click to view showcase
println!("My super cool code.");

@JMS55 JMS55 added the A-Assets Load files from disk to use for things like images, models, and sounds label Mar 29, 2026
@github-project-automation github-project-automation bot moved this to Needs SME Triage in Assets Mar 29, 2026
@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Mar 29, 2026
@beicause
Copy link
Copy Markdown
Contributor

beicause commented Apr 1, 2026

FYI I'm working on a basisu asset processor in beicause/bevy_basisu_loader#33

Comment thread crates/bevy_image/src/compressed_image_saver.rs Outdated
Comment thread crates/bevy_image/src/compressed_image_saver.rs
return Err(CompressedImageSaverError::UninitializedImage);
};

if image.texture_descriptor.mip_level_count != 1 {
Copy link
Copy Markdown

@cwfitzgerald cwfitzgerald Apr 12, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note ctt can deal with any source amount of mips

stride: image.width() * bytes_per_pixel,
format: input_format,
color_space,
alpha: ctt::AlphaMode::Straight, // TODO: User-configurable?
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this should be user configurable.

container: ctt::Container::Ktx2,
quality: ctt::Quality::default(),
output_color_space: None,
output_alpha: None,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The correct default here is Some(premultiplied), though should be user configurable

Comment thread crates/bevy_image/src/compressed_image_saver.rs Outdated
Comment thread crates/bevy_image/src/compressed_image_saver.rs Outdated
Comment thread crates/bevy_internal/Cargo.toml Outdated
Copy link
Copy Markdown
Contributor

@andriyDev andriyDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM. We're just calling another lib!

I haven't tested this locally, but the code looks fine (barring one complaint).

})
}

#[cfg(feature = "compressed_image_saver_universal")]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am really not a fan of this. Saying we just straight up can't compile either case if both features are enabled kinda sucks.

WDYT about just having two CompressedImageSavers? CompressedImageSaverCtt and CompressedImageSaverUniversal? Then just pick one in the plugin? This way if users want to use the old compression, they can. You could also have a wrapper CompressedImageSaver for compatibility (which internally holds one of the backends), so that we keep old meta files working.

I am personally not a fan of mutually exclusive feature flags, since it means I can't just test with --all-features.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally the idea was that you'd always be using CompressedImageSaver (same meta files), and then depending on what platform you want to target, you'd enable different backends.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can use feature gate within a custom asset processor instead of asset savers, instead of using LoadTransformAndSave.

Comment thread _release-content/migration-guides/compressed_image_saver.md Outdated
Comment thread crates/bevy_image/src/compressed_image_saver.rs Outdated
Comment thread crates/bevy_image/src/compressed_image_saver.rs
///
/// # ASTC override
///
/// Set the `BEVY_COMPRESSED_IMAGE_SAVER_ASTC` environment variable to compress into ASTC
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the default should be based on the target. Like android and ios should default to ASTC and other platforms default to BCn.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about this, but it would be pretty standard to compile for one platform on another, e.g. in CI, right?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where does this code run? If you're cross compiling you should know the target you're cross compiling to.

Comment on lines +81 to +82
/// increases GPU cache hits, improving rendering performance. Input images must have a
/// `mip_level_count` of 1 (i.e., no pre-existing mip chain); the compressor will produce
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this isn't true unless you force it? ctt will handle this

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to generate the Surfaces in the correct order from Bevy's data, which I didn't feel like figuring out...

Comment thread crates/bevy_image/src/compressed_image_saver.rs Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Assets Load files from disk to use for things like images, models, and sounds C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged

Projects

Status: Needs SME Triage

Development

Successfully merging this pull request may close these issues.

Improve CompressedImageSaver

5 participants